Skip to content

Add self param to array object properties #464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

IsaacBreen
Copy link
Contributor

No description provided.

@leofang
Copy link
Contributor

leofang commented Jul 18, 2022

I am not typing expert but I think we need TypeVar(T, bound='array') now (PEP 484), and in the future Self (PEP 673). The current approach would not work for subclasses. My vote would go for the status quo (no type hint for self) but let's wait for others to chime in.

@asmeurer
Copy link
Member

array is what the other methods use so keeping that here is fine. We can open another issue about using a different type hint for self.

@rgommers
Copy link
Member

CI failure is unrelated, let's retry that.

@rgommers rgommers closed this Jul 27, 2022
@rgommers rgommers reopened this Jul 27, 2022
@leofang
Copy link
Contributor

leofang commented Jul 27, 2022

Sorry, so this PR does two things:

  1. It fixes the missing self to the array methods (which I overlooked)
  2. It adds a type hint for self

We can open another issue about using a different type hint for self.

Let's do that.

@rgommers
Copy link
Member

Okay, CI is unrelated, I'll open a separate issue for that too.

@leofang
Copy link
Contributor

leofang commented Jul 27, 2022

Yeah the CI failure is likely a permission issue that we also hit before.

@rgommers
Copy link
Member

The type hint for self is one instance of a general pattern; same as for scalars we use int and not anything that ducktypes with int (which can be type-able by SupportsInt or some such thing).

@rgommers rgommers added the bug Something isn't working. label Jul 27, 2022
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like we're happy here, in it goes.

@rgommers rgommers merged commit bc49588 into data-apis:main Jul 27, 2022
@kgryte kgryte added this to the v2022 milestone Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants